Feature: Auto-update client#4256
Conversation
mohamed-essam
force-pushed
the
feat/auto-upgrade
branch
4 times, most recently
from
76b9397 to
9d44077
Compare
|
@lixmal All comments should be resolved, will need to conduct another round of testing however, so will keep the PR as Draft until testing is complete |
mohamed-essam
force-pushed
the
feat/auto-upgrade
branch
5 times, most recently
from
03eb50e to
d15b3e5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an auto-update client feature that allows the management server to control client version updates. The feature enables administrators to specify a target version for clients, which can be "latest", "disabled", or a specific semantic version.
- Added AutoUpdateVersion field to account settings with validation for "latest", "disabled", or semantic version format
- Implemented platform-specific update mechanisms for Windows, macOS, Linux, and FreeBSD
- Added update manager that monitors version changes and triggers updates when client version is older than target
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| version/update.go | Made LatestAvailable field public for external access |
| management/server/types/settings.go | Added AutoUpdateVersion field to Settings struct |
| management/server/token_mgr.go | Added AutoUpdateVersion to TURN/relay token updates |
| management/server/peer.go | Added AutoUpdateVersion to peer deletion updates |
| management/server/http/handlers/accounts/ | Added API handlers and validation for AutoUpdateVersion |
| management/server/http/api/ | Added AutoUpdateVersion to API types and OpenAPI spec |
| management/server/grpcserver.go | Added AutoUpdateVersion to sync response |
| management/server/activity/codes.go | Added activity tracking for AutoUpdateVersion updates |
| management/server/account.go | Added handling for AutoUpdateVersion setting changes |
| management/proto/management.proto | Added autoUpdateVersion field to SyncResponse |
| client/internal/updatemanager/ | Implemented platform-specific update mechanisms |
| client/internal/engine.go | Integrated update manager with sync response handling |
mohamed-essam
force-pushed
the
feat/auto-upgrade
branch
from
d15b3e5 to
5ba5b40
Compare
lixmal
left a comment
lixmal
left a comment
There was a problem hiding this comment.
Have you put any thoughts into how we could do a rollback if the new version breaks connectivity?
Based on my discussions with @mlsmaycon , the upgrade mechanism is forward-moving only |
|
@lixmal If it's alright with you, I will mark the PR as no documentation needed for the time being as the dashboard changes are not yet done, as soon as dashboard can modify the auto upgrade version I will open a PR in docs |
|
Very much looking forward to this feature. Thanks for the good work. |
| u.cancel() | ||
| if u.update != nil { | ||
| u.update.StopWatch() | ||
| u.update = nil |
There was a problem hiding this comment.
You set u.update = nil in Stop() while updateLoop or handleUpdate could still try to access it.
There was a problem hiding this comment.
This is only referenced inside handleUpdate once inside a mutex, so I used this to prevent a race and also added a check so that if u.update is nil it should return
| return false | ||
| } | ||
|
|
||
| if time.Since(u.lastTrigger) < 5*time.Minute { |
There was a problem hiding this comment.
I am not sure but we can miss a potential upgrade if we just ignore. What is the goal with this condition?
There was a problem hiding this comment.
This is so it doesn't spam retrying to upgrade with any trigger since each failure will show an error to the user, so this is just a way to backoff failures
Modify client-side behavior
- fix development version handling - add log lines
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an automatic update subsystem and integrates it end-to-end: new UpdateManager with platform triggers, wiring into Engine startup and sync flows, management API/proto/OpenAPI changes to carry auto-update settings, UI progress support, and tests plus version-fetcher refactor. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Engine
participant Management
participant UpdateMgr as UpdateManager
participant Platform
participant State
User->>Engine: Start
Engine->>Management: Login / Sync request
Management-->>Engine: SyncResponse (NetworkMap with PeerConfig.autoUpdate)
alt peerConfig.autoUpdate present
Engine->>Engine: InitialUpdateHandling(autoUpdateSettings)
Engine->>UpdateMgr: Start / SetVersion(autoUpdate.version)
UpdateMgr->>State: Load persisted state
UpdateMgr->>UpdateMgr: evaluate shouldUpdate()
alt shouldUpdate == true
UpdateMgr->>Platform: triggerUpdate(targetVersion)
Platform-->>UpdateMgr: install result
UpdateMgr->>State: persist update outcome
end
end
Engine-->>User: Running
sequenceDiagram
participant NetworkSync
participant Engine
participant UpdateMgr
NetworkSync->>Engine: handleSync(networkMap with PeerConfig)
alt networkMap.PeerConfig present
Engine->>Engine: handleAutoUpdateVersion(autoUpdateSettings, initialCheck=false)
Engine->>UpdateMgr: SetVersion / maybe Start
UpdateMgr->>UpdateMgr: updateLoop or immediate handleUpdate
end
Engine-->>NetworkSync: Sync processed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
version/update.go (1)
175-179: Guard againstlatestAvailablebeing nil.On the very first successful fetch,
u.latestAvailableis still nil, so calling.Equaldereferences nil and panics, killing the watcher. Add a nil check before comparing.- if u.latestAvailable.Equal(latestAvailable) { + if u.latestAvailable != nil && u.latestAvailable.Equal(latestAvailable) { return false }
🧹 Nitpick comments (5)
shared/management/proto/management.proto (1)
274-281: Tighten semantics in AutoUpdateSettings comments (values, policy timing, presence)Add explicit value contract for version and clarify update policy timing. Suggested comment-only change:
message AutoUpdateSettings { - string version = 1; - /* - alwaysUpdate = true → Updates happen automatically in the background - alwaysUpdate = false → Updates only happen when triggered by a peer connection - */ - bool alwaysUpdate = 2; + // Version directive: "latest", "disabled", or a concrete semver (e.g., "0.50.1"). + // Clients SHOULD treat unknown strings as "disabled" and MUST NOT auto-update. + string version = 1; + // Update policy: + // - true → background auto-update (periodic checks without user action) + // - false → update only on management login/sync handshake + // Presence note: omit the entire AutoUpdateSettings message to inherit server defaults. + bool alwaysUpdate = 2; }client/internal/updatemanager/update_freebsd.go (1)
7-10: FreeBSD update implementation pending.The
triggerUpdatemethod is currently a stub. According to PR comments, platform-specific installer logic will handle update execution. Ensure this TODO is tracked.Would you like me to open an issue to track the FreeBSD implementation?
client/internal/connect.go (1)
283-285: Consider defensive nil check for PeerConfig.The code checks if
AutoUpdateis non-nil, butloginResp.PeerConfigitself could potentially be nil. Consider adding a nil check forPeerConfigbefore accessing itsAutoUpdatefield to prevent potential panic.Apply this diff to add defensive nil check:
- if loginResp.PeerConfig != nil && loginResp.PeerConfig.AutoUpdate != nil { - c.engine.InitialUpdateHandling(loginResp.PeerConfig.AutoUpdate) - } + if peerConfig := loginResp.GetPeerConfig(); peerConfig != nil && peerConfig.GetAutoUpdate() != nil { + c.engine.InitialUpdateHandling(peerConfig.GetAutoUpdate()) + }client/internal/updatemanager/update_linux.go (1)
7-10: Linux update implementation pending.The
triggerUpdatemethod is a stub awaiting implementation. Based on PR discussion, platform-specific installers (e.g., apt) will handle the actual update mechanics.Would you like me to open an issue to track the Linux implementation alongside FreeBSD?
management/server/grpcserver.go (1)
774-776: AutoUpdate configuration wired correctly.The AutoUpdate field is properly populated from account settings. Note that a non-nil
AutoUpdateSettingsstruct is always created even whenAutoUpdateVersionis an empty string.If you want to distinguish between "no auto-update configured" vs "update to latest", consider conditionally creating the AutoUpdate struct:
+ var autoUpdate *proto.AutoUpdateSettings + if settings.AutoUpdateVersion != "" { + autoUpdate = &proto.AutoUpdateSettings{ + Version: settings.AutoUpdateVersion, + } + } return &proto.PeerConfig{ Address: fmt.Sprintf("%s/%d", peer.IP.String(), netmask), SshConfig: &proto.SSHConfig{SshEnabled: peer.SSHEnabled}, Fqdn: fqdn, RoutingPeerDnsResolutionEnabled: settings.RoutingPeerDNSResolutionEnabled, LazyConnectionEnabled: settings.LazyConnectionEnabled, - AutoUpdate: &proto.AutoUpdateSettings{ - Version: settings.AutoUpdateVersion, - }, + AutoUpdate: autoUpdate, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (23)
client/internal/connect.go(1 hunks)client/internal/engine.go(6 hunks)client/internal/updatemanager/manager.go(1 hunks)client/internal/updatemanager/manager_test.go(1 hunks)client/internal/updatemanager/update_darwin.go(1 hunks)client/internal/updatemanager/update_freebsd.go(1 hunks)client/internal/updatemanager/update_js.go(1 hunks)client/internal/updatemanager/update_linux.go(1 hunks)client/internal/updatemanager/update_windows.go(1 hunks)client/ui/client_ui.go(9 hunks)management/internals/server/server.go(1 hunks)management/server/account.go(3 hunks)management/server/activity/codes.go(2 hunks)management/server/grpcserver.go(1 hunks)management/server/http/handlers/accounts/accounts_handler.go(6 hunks)management/server/http/handlers/accounts/accounts_handler_test.go(5 hunks)management/server/peer.go(1 hunks)management/server/types/settings.go(2 hunks)shared/management/http/api/openapi.yml(1 hunks)shared/management/http/api/types.gen.go(1 hunks)shared/management/proto/management.proto(1 hunks)version/update.go(3 hunks)version/update_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
client/internal/connect.go (1)
shared/management/proto/management.pb.go (3)
PeerConfig(1637-1655)PeerConfig(1670-1670)PeerConfig(1685-1687)
management/internals/server/server.go (1)
version/update.go (1)
NewUpdateAndStart(53-58)
client/internal/updatemanager/update_freebsd.go (1)
client/internal/updatemanager/manager.go (1)
UpdateManager(46-64)
management/server/account.go (2)
management/server/types/settings.go (1)
Settings(10-58)management/server/activity/codes.go (1)
AccountAutoUpdateVersionUpdated(183-183)
client/internal/updatemanager/update_darwin.go (1)
client/internal/updatemanager/manager.go (1)
UpdateManager(46-64)
client/internal/updatemanager/update_js.go (1)
client/internal/updatemanager/manager.go (1)
UpdateManager(46-64)
client/internal/updatemanager/manager_test.go (2)
client/internal/updatemanager/manager.go (3)
UpdateManager(46-64)UpdateInterface(29-35)NewUpdateManager(66-77)client/internal/peer/status.go (1)
NewRecorder(222-233)
version/update_test.go (1)
version/update.go (1)
NewUpdateAndStart(53-58)
client/internal/updatemanager/update_windows.go (1)
client/internal/updatemanager/manager.go (1)
UpdateManager(46-64)
management/server/http/handlers/accounts/accounts_handler_test.go (2)
management/server/types/account.go (1)
AccountSettings(117-119)shared/management/http/api/types.gen.go (1)
AccountSettings(293-336)
management/server/peer.go (1)
shared/management/proto/management.pb.go (3)
PeerConfig(1637-1655)PeerConfig(1670-1670)PeerConfig(1685-1687)
client/internal/engine.go (2)
client/internal/updatemanager/manager.go (2)
UpdateManager(46-64)NewUpdateManager(66-77)shared/management/proto/management.pb.go (12)
AutoUpdateSettings(1745-1753)AutoUpdateSettings(1768-1768)AutoUpdateSettings(1783-1785)SyncResponse(388-405)SyncResponse(420-420)SyncResponse(435-437)NetworkMap(1802-1832)NetworkMap(1847-1847)NetworkMap(1862-1864)PeerConfig(1637-1655)PeerConfig(1670-1670)PeerConfig(1685-1687)
management/server/grpcserver.go (3)
shared/management/proto/management.pb.go (18)
AutoUpdateSettings(1745-1753)AutoUpdateSettings(1768-1768)AutoUpdateSettings(1783-1785)NetworkMap(1802-1832)NetworkMap(1847-1847)NetworkMap(1862-1864)Checks(3046-3052)Checks(3067-3067)Checks(3082-3084)SyncResponse(388-405)SyncResponse(420-420)SyncResponse(435-437)PeerConfig(1637-1655)PeerConfig(1670-1670)PeerConfig(1685-1687)DNSConfig(2551-2560)DNSConfig(2575-2575)DNSConfig(2590-2592)management/server/types/network.go (2)
NetworkMap(32-41)Network(109-118)management/server/types/settings.go (2)
Settings(10-58)ExtraSettings(86-103)
client/ui/client_ui.go (2)
version/update.go (1)
NewUpdateAndStart(53-58)client/proto/daemon.pb.go (3)
SystemEvent(3424-3435)SystemEvent(3448-3448)SystemEvent(3463-3465)
client/internal/updatemanager/manager.go (3)
version/version.go (1)
NetbirdVersion(18-20)version/update.go (1)
NewUpdate(38-51)client/proto/daemon.pb.go (3)
SystemEvent_INFO(94-94)SystemEvent_SYSTEM(150-150)SystemEvent_ERROR(96-96)
management/server/http/handlers/accounts/accounts_handler.go (4)
shared/management/http/api/types.gen.go (1)
PutApiAccountsAccountIdJSONRequestBody(1936-1936)management/server/types/settings.go (2)
Settings(10-58)ExtraSettings(86-103)management/server/context/auth.go (1)
GetUserAuthFromContext(47-52)shared/management/http/util/util.go (2)
WriteError(84-120)WriteErrorResponse(70-80)
client/internal/updatemanager/update_linux.go (1)
client/internal/updatemanager/manager.go (1)
UpdateManager(46-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Client / Unit
🔇 Additional comments (11)
shared/management/proto/management.proto (1)
270-272: <!-- [scratchpad_start]
[task review_proto_autoUpdate_1/1]
Verify all PeerConfig builders properly initialize autoUpdate field from server settings.[observations]
- toPeerConfig function defined at management/server/grpcserver.go:765-777
- AutoUpdate field EXPLICITLY initialized at lines 774-776:
AutoUpdate: &proto.AutoUpdateSettings{Version: settings.AutoUpdateVersion,}- toPeerConfig is called from 3 production locations:
- management/server/peer.go:1649 (passing settings)
- management/server/grpcserver.go:650 (passing settings)
- management/server/grpcserver.go:782 (passing settings in toSyncResponse)
- management/server/grpcserver.go:787 (passing settings in NetworkMap)
- All call sites pass the required settings parameter
- Other direct PeerConfig constructions (peer.go:1641, engine_test.go) are in error/empty scenarios or tests, not primary builders
- Field is non-nil and populated from settings.AutoUpdateVersion
[analysis]
- Primary concern is addressed: AutoUpdate IS populated in the main builder
- Field uses correct source: settings.AutoUpdateVersion
- All production code paths go through toPeerConfig which ensures population
- No nil risks on client for normal flows
- Code quality: Proper initialization pattern, settings-driven
[done]
✅ Verification complete: AutoUpdate is properly initialized from server settings in all production PeerConfig builders. Original concern validated.
[scratchpad_end]AutoUpdate field is properly populated in all PeerConfig builders
Verification confirms that the
toPeerConfigfunction (management/server/grpcserver.go:765–777) correctly initializesautoUpdatefromsettings.AutoUpdateVersion(lines 774–776). All three production call sites pass the required settings parameter, ensuring clients always receive the field populated. No nil surprises.management/server/peer.go (1)
1649-1654: Verified: toPeerConfig is safe and AutoUpdate correctly populatedConfirmed both concerns:
toPeerConfigis a pure data mapper with no store lookups—only accessor calls on passed parameters (safe post-DeletePeer).AutoUpdate.Versionis correctly derived fromsettings.AutoUpdateVersion.The change is sound.
shared/management/http/api/types.gen.go (1)
294-296: Now let me search the codebase for the constants and any defaulting logic:Based on my verification of the code, I can now generate the rewritten review comment:
Ensure empty AutoUpdateVersion values are handled correctly per design intent
Server-side validation is already implemented at
management/server/http/handlers/accounts/accounts_handler.golines 210-219. The handler validates that non-empty values match either "latest", "disabled", or a valid semantic version.However, empty strings currently pass through without error and are not defaulted to "latest" (line 216 condition allows empty values). Confirm whether this is intentional: if empty values should default to "latest", add that logic at line 216. The codegen file remains untouched as required.
shared/management/http/api/openapi.yml (1)
148-152: The suggested approach won't produce the expected result; oapi-codegen v1.11.1 does not convert OpenAPI pattern constraints to Go validation struct tags.The review comment assumes that adding a
patternto the OpenAPI schema and regenerating types will produce validation tags in the generated Go code. However, oapi-codegen v1.11.1 does not automatically convert OpenAPI pattern constraints into Go validation struct tags.What's correct: Adding a pattern to
openapi.ymlforauto_update_versionis beneficial for API schema documentation and API-level validation. The proposed regex and description improvements are sound.What's incorrect: The verification premise—that running codegen will add validation constraints to
types.gen.go—won't happen. The pattern will remain an OpenAPI schema artifact and will not appear in the generated Go struct tags.To actually add Go-level validation, you'd need one of these approaches:
- Use
x-oapi-codegen-extra-tagsin the OpenAPI schema to inject validation tags directly- Manual validation in handlers or middleware
- Use a different code generator with validation support
The review comment conflates OpenAPI schema constraints with Go struct validation, which are separate concerns in oapi-codegen.
Likely an incorrect or invalid review comment.
management/server/types/settings.go (2)
56-57: LGTM! AutoUpdateVersion field added correctly.The new field is properly defined with an appropriate default value. The gorm tag
default:'latest'provides sensible default behavior for auto-update configuration.
78-78: Copy method properly updated.The AutoUpdateVersion field is correctly included in the Copy method, maintaining consistency with the struct definition.
management/server/activity/codes.go (2)
183-183: Activity constant added correctly.The new
AccountAutoUpdateVersionUpdatedactivity follows the established pattern and uses the next sequential value (91).
295-295: Activity mapping entry is correct.The mapping entry follows the naming convention and provides clear, descriptive text for the auto-update version change activity.
management/internals/server/server.go (1)
186-186: Update initialization correctly refactored.The change from
NewUpdatetoNewUpdateAndStartaligns with the new API that automatically starts the version fetcher goroutine. The existingStopWatch()call in theStop()method (line 216) will properly clean up the started goroutine.version/update_test.go (1)
26-26: Tests correctly updated for new API.All test functions have been updated to use
NewUpdateAndStartinstead ofNewUpdate, aligning with the refactored initialization pattern. The existingStopWatch()cleanup calls ensure proper goroutine termination.Also applies to: 51-51, 76-76
management/server/grpcserver.go (1)
784-787: NetworkMap correctly includes PeerConfig.The PeerConfig field is properly added to the NetworkMap proto response, enabling clients to receive auto-update configuration alongside network topology. The spacing adjustments improve readability.
| err = cmd.Start() | ||
| if err != nil { | ||
| return fmt.Errorf("error running pkg file: %w", err) | ||
| } | ||
| err = cmd.Process.Release() | ||
|
|
||
| return err |
There was a problem hiding this comment.
Wait for the pkg installer to finish before returning.
We start the installer and immediately Release the process, so triggerUpdate reports success even if the install fails (or is killed by the 1‑minute deadline). We also lose the installer’s exit status entirely. Please run the command synchronously so we only return after the pkg has completed (and propagate any failure).
Apply this diff:
- err = cmd.Start()
- if err != nil {
- return fmt.Errorf("error running pkg file: %w", err)
- }
- err = cmd.Process.Release()
-
- return err
+ if err := cmd.Run(); err != nil {
+ return fmt.Errorf("error running pkg file: %w", err)
+ }
+ return nil🤖 Prompt for AI Agents
In client/internal/updatemanager/update_darwin.go around lines 44 to 50, the
code currently starts the pkg installer and immediately calls Process.Release
which returns before the installer exits and drops its exit status; replace this
with a synchronous run (either use cmd.Run() or call cmd.Start() followed by
cmd.Wait()) so the function only returns after the installer completes, capture
and return any error from Run/Wait (wrapping with fmt.Errorf("%w") if needed),
and remove the immediate Process.Release so the installer's exit status is
propagated.
| fileInfo, err := os.Stat("/opt/homebrew/Library/Taps/netbirdio/homebrew-tap/") | ||
| if err != nil { | ||
| return fmt.Errorf("error getting homebrew installation path info: %w", err) | ||
| } | ||
|
|
||
| fileSysInfo, ok := fileInfo.Sys().(*syscall.Stat_t) | ||
| if !ok { | ||
| return fmt.Errorf("error checking file owner, sysInfo type is %T not *syscall.Stat_t", fileInfo.Sys()) | ||
| } | ||
|
|
||
| // Get username from UID | ||
| installer, err := user.LookupId(fmt.Sprintf("%d", fileSysInfo.Uid)) | ||
| if err != nil { | ||
| return fmt.Errorf("error looking up brew installer user: %w", err) | ||
| } | ||
| userName := installer.Name | ||
| // Get user HOME, required for brew to run correctly | ||
| // https://github.com/Homebrew/brew/issues/15833 | ||
| homeDir := installer.HomeDir | ||
| // Homebrew does not support installing specific versions | ||
| // Thus it will always update to latest and ignore targetVersion | ||
| upgradeArgs := []string{"-u", userName, "/opt/homebrew/bin/brew", "upgrade", "netbirdio/tap/netbird"} | ||
| // Check if netbird-ui is installed | ||
| cmd := exec.CommandContext(ctx, "brew", "info", "--json", "netbirdio/tap/netbird-ui") | ||
| err = cmd.Run() | ||
| if err == nil { | ||
| // netbird-ui is installed | ||
| upgradeArgs = append(upgradeArgs, "netbirdio/tap/netbird-ui") | ||
| } | ||
| cmd = exec.CommandContext(ctx, "sudo", upgradeArgs...) |
There was a problem hiding this comment.
Handle Homebrew installations outside /opt/homebrew.
The tap path and brew executable are hard-coded to /opt/homebrew. On Intel macOS the default prefix is /usr/local/Homebrew (brew lives in /usr/local/bin), so os.Stat fails and the upgrade always returns the error path—auto-update will never run there. We should detect the actual Homebrew prefix (e.g. sudo -u <user> brew --prefix) and derive both the tap directory and brew binary from it, with a sane error if no prefix is found.
One possible fix:
- fileInfo, err := os.Stat("/opt/homebrew/Library/Taps/netbirdio/homebrew-tap/")
- if err != nil {
- return fmt.Errorf("error getting homebrew installation path info: %w", err)
- }
+ brewPrefixCmd := exec.CommandContext(ctx, "sudo", "-u", userName, "brew", "--prefix")
+ brewPrefixCmd.Env = append(os.Environ(), "HOME="+homeDir)
+ prefixBytes, err := brewPrefixCmd.Output()
+ if err != nil {
+ return fmt.Errorf("error determining homebrew prefix: %w", err)
+ }
+ brewPrefix := strings.TrimSpace(string(prefixBytes))
+ tapDir := filepath.Join(brewPrefix, "Library", "Taps", "netbirdio", "homebrew-tap")
+
+ fileInfo, err := os.Stat(tapDir)
+ if err != nil {
+ return fmt.Errorf("error getting homebrew installation path info: %w", err)
+ }
@@
- upgradeArgs := []string{"-u", userName, "/opt/homebrew/bin/brew", "upgrade", "netbirdio/tap/netbird"}
+ brewBin := filepath.Join(brewPrefix, "bin", "brew")
+ upgradeArgs := []string{"-u", userName, brewBin, "upgrade", "netbirdio/tap/netbird"}Remember to add the necessary imports (filepath).
Based on learnings
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/internal/updatemanager/update_darwin.go around lines 56 to 85, the
code currently hard-codes /opt/homebrew for the tap path and brew binary which
fails on Intel macs; change it to detect the actual Homebrew prefix for the
installer user (run sudo -u <user> brew --prefix via exec.CommandContext,
capture and trim stdout) and derive the tap directory as filepath.Join(prefix,
"Library", "Taps", "netbirdio", "homebrew-tap") and the brew binary as
filepath.Join(prefix, "bin", "brew"); if the prefix command fails or returns
empty, return a clear error; update subsequent os.Stat and brew invocations to
use these derived paths (use exec.CommandContext(ctx, "sudo", "-u", userName,
derivedBrewPath, "info", "--json", "netbirdio/tap/netbird-ui") and similarly for
upgrade), and add the filepath import. Ensure errors are wrapped with context
and fall back only to a clear failure rather than assuming /opt/homebrew.
| } | ||
| if req.Settings.AutoUpdateVersion != nil { | ||
| _, err := goversion.NewSemver(*req.Settings.AutoUpdateVersion) | ||
| if *req.Settings.AutoUpdateVersion == autoUpdateLatestVersion || | ||
| *req.Settings.AutoUpdateVersion == disableAutoUpdate || | ||
| err == nil { | ||
| returnSettings.AutoUpdateVersion = *req.Settings.AutoUpdateVersion | ||
| } else if *req.Settings.AutoUpdateVersion != "" { | ||
| return nil, fmt.Errorf("invalid AutoUpdateVersion") | ||
| } |
There was a problem hiding this comment.
Return a status.InvalidArgument error for invalid versions.
Returning fmt.Errorf makes util.WriteError emit a 500 “internal server error” to clients who send a bad version. We should surface a 422 InvalidArgument instead.
- return nil, fmt.Errorf("invalid AutoUpdateVersion")
+ return nil, status.Errorf(status.InvalidArgument, "invalid auto-update version")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| if req.Settings.AutoUpdateVersion != nil { | |
| _, err := goversion.NewSemver(*req.Settings.AutoUpdateVersion) | |
| if *req.Settings.AutoUpdateVersion == autoUpdateLatestVersion || | |
| *req.Settings.AutoUpdateVersion == disableAutoUpdate || | |
| err == nil { | |
| returnSettings.AutoUpdateVersion = *req.Settings.AutoUpdateVersion | |
| } else if *req.Settings.AutoUpdateVersion != "" { | |
| return nil, fmt.Errorf("invalid AutoUpdateVersion") | |
| } | |
| } | |
| if req.Settings.AutoUpdateVersion != nil { | |
| _, err := goversion.NewSemver(*req.Settings.AutoUpdateVersion) | |
| if *req.Settings.AutoUpdateVersion == autoUpdateLatestVersion || | |
| *req.Settings.AutoUpdateVersion == disableAutoUpdate || | |
| err == nil { | |
| returnSettings.AutoUpdateVersion = *req.Settings.AutoUpdateVersion | |
| } else if *req.Settings.AutoUpdateVersion != "" { | |
| return nil, status.Errorf(status.InvalidArgument, "invalid auto-update version") | |
| } |
🤖 Prompt for AI Agents
management/server/http/handlers/accounts/accounts_handler.go around lines 209 to
218: the code currently returns fmt.Errorf("invalid AutoUpdateVersion") for bad
versions which causes util.WriteError to emit a 500; change the error return to
a gRPC InvalidArgument error so clients receive a 422/invalid-argument. Replace
the fmt.Errorf(...) with a status.Errorf(codes.InvalidArgument, "invalid
AutoUpdateVersion: %s", *req.Settings.AutoUpdateVersion) (and add/import
"google.golang.org/grpc/status" and "google.golang.org/grpc/codes" if not
already present).
|
Describe your changes
Issue ticket number and link
Stack
Checklist
Summary by CodeRabbit
New Features
Server & Sync
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.